Support for HTTP/3 (client side) by reta · Pull Request #21718 · opensearch-project/OpenSearch

reta · 2026-05-18T16:03:59Z

Description

Support for HTTP/3 (client side) by introducing alternative, zero dependencies HTTP client built on top of JDK's HttpClient.

Motivation

Recently we have introduced an experimental HTTP/3 support on the served side. The client side turned out to be more complicated since the Apache HttpClient 5 that we rely on for rest client transport does not support HTTP/3 (and unluckily will in nearest future), see please https://issues.apache.org/jira/browse/HTTPCORE-793. This limitation leads to uneasy options:

no support of the HTTP/3 on client side
provide alternative client implementation to support HTTP/3 (and HTTP/1.1, HTTP/2, ... )

Alternative base clients

There are basically two production ready clients that fit into OpenSearch technical stack:

Netty Client (and Reactor Netty that is built on top of it)
OpenJDK HttpClient that supports HTTP/3 as of JDK-26 [1], [2]

OpenJDK looks particularly appealing since it requires no dependencies vs Netty that provides the foundation but requires more implementation work. Although our baseline is JDK-21, the applications could use JDK-26 (or later) and get HTTP/3 support for free, no changes required on OpenSearch side. Contrary, Netty's HTTP/3 implementation is built on top of native library (https://github.com/cloudflare/quiche) and is not available on many platforms.

Do we need new HTTP client?

The Rest Client is considered deprecated for a long time (we recommend to use opensearch-java), so we could consider it to be an internal change. Also, we stuck on Java 8 baseline for opensearch-java and HttpClient is only available in JDK-11, adding a support for it is possible as multi-release jar and will be added accordingly.

[1] #21170
[2] https://openjdk.org/jeps/517

Implementation Notes

The new client borrows a large chunk of implementation from the existing RestClient and preserves the attribution (license headers). The new client though has no dependencies besides JDK itself.

Related Issues

Closes #20634

Check List

Functionality includes testing.
API changes companion pull request created, if applicable.
Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

github-actions · 2026-05-18T16:05:26Z

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit dcf871e.

Path	Line	Severity	Description
client/rest-http-client/build.gradle	38	high	New dependency added: io.projectreactor:reactor-core. All new dependencies must be verified by maintainers per mandatory supply chain flagging rule.
client/rest-http-client/build.gradle	39	high	New dependency added: org.reactivestreams:reactive-streams. All new dependencies must be verified by maintainers per mandatory supply chain flagging rule.
client/rest-http-client/build.gradle	33	high	New FIPS runtime dependency: org.bouncycastle:bc-fips. Cryptographic library additions require artifact authenticity verification.
client/rest-http-client/build.gradle	34	high	New FIPS runtime dependency: org.bouncycastle:bctls-fips. TLS library additions require artifact authenticity verification.
client/rest-http-client/build.gradle	35	high	New FIPS runtime dependency: org.bouncycastle:bcutil-fips. Cryptographic utility library additions require artifact authenticity verification.
client/rest-http-client/build.gradle	30	high	New dependency added: commons-codec:commons-codec. All new dependencies must be verified by maintainers per mandatory supply chain flagging rule.
client/rest-http-client/build.gradle	31	high	New dependency added: commons-logging:commons-logging. All new dependencies must be verified by maintainers per mandatory supply chain flagging rule.
client/rest-http-client/build.gradle	32	high	New dependency added: org.slf4j:slf4j-api. All new dependencies must be verified by maintainers per mandatory supply chain flagging rule.
client/rest-http-client/licenses/reactor-core-3.8.5.jar.sha1	1	high	New SHA1 checksum file added for reactor-core-3.8.5. Maintainers must independently verify this hash matches the expected artifact from the official release.
client/rest-http-client/licenses/bc-fips-2.1.2.jar.sha1	1	high	New SHA1 checksum file added for bc-fips-2.1.2. Maintainers must independently verify this hash matches the expected BouncyCastle FIPS artifact.
settings.gradle	31	high	New Gradle subproject 'client:rest-http-client' registered. This activates all build.gradle dependency declarations in the new module, including all newly introduced third-party libraries.

The table above displays the top 10 most important findings.

Total: 11 | Critical: 0 | High: 11 | Medium: 0 | Low: 0

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

reta · 2026-05-18T16:06:07Z

@andrross @cwperks looking for early feedback folks. The pull request is not in ready state yet, mostly looking into "does it make sense or not" perspective, since we reach some limits of the client HTTP libraries we use. Thanks a lot, would really appreciate thoughts.

github-actions · 2026-05-18T16:07:53Z

PR Reviewer Guide 🔍

(Review updated until commit `b5d3e36`)

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ Recommended focus areas for review Possible Issue In `performRequest`, if `convertResponse` throws a `ResponseException` that is not retryable (status code not 502/503/504), the exception is thrown immediately without marking the node as alive via `onResponse(node)`. However, `convertResponse` already calls `onResponse(node)` before throwing in this scenario (line 370). This is correct. But if `convertResponse` throws any other exception (like `WarningFailureException` at line 359), the node is never marked as alive or dead, potentially leaving it in an inconsistent state in the denylist. The same issue exists in the async path at line 461 where any exception from `convertResponse` is caught and passed to `onDefinitiveFailure` without updating node state. private Response performRequest(final Iterator<Node> nodes, final InternalRequest request, Exception previousException) throws IOException { Node node = nodes.next(); RequestContext<List<ByteBuffer>> context = request.createContextForNextAttempt(node); HttpResponse<List<ByteBuffer>> httpResponse; try { httpResponse = client.send(context.requestProducer(), context.asyncResponseConsumer()); } catch (Exception e) { RequestLogger.logFailedRequest(logger, request.httpRequest, context.node(), e); onFailure(context.node()); Exception cause = extractAndWrapCause(e); addSuppressedException(previousException, cause); if (nodes.hasNext()) { return performRequest(nodes, request, cause); } if (cause instanceof IOException) { throw (IOException) cause; } if (cause instanceof RuntimeException) { throw (RuntimeException) cause; } throw new IllegalStateException("unexpected exception type: must be either RuntimeException or IOException", cause); } ResponseOrResponseException responseOrResponseException = convertResponse(request, context.node(), httpResponse); if (responseOrResponseException.responseException == null) { return responseOrResponseException.response; } addSuppressedException(previousException, responseOrResponseException.responseException); if (nodes.hasNext()) { return performRequest(nodes, request, responseOrResponseException.responseException); } throw responseOrResponseException.responseException; } Possible Issue In `streamRequest` (line 307-339), if `convertResponse` throws an exception other than `ResponseException`, it is propagated via `Mono.error` without calling `onFailure(node)` to update the denylist. This means a node that encounters an error (e.g., `WarningFailureException`) will not be denylisted, and subsequent requests may continue to hit the same node and fail repeatedly. The non-streaming async path has a similar issue at line 460-462. final Mono<HttpResponse<Flow.Publisher<List<ByteBuffer>>>> publisher = Mono.fromCompletionStage(future).flatMap(response -> { try { final ResponseOrResponseException responseOrResponseException = convertResponse(request, node, response); if (responseOrResponseException.responseException == null) { return Mono.just(response); } else { if (nodes.hasNext()) { return Mono.from(streamRequest(nodes.next(), nodes, request)); } else { return Mono.error(responseOrResponseException.responseException); } } } catch (final Exception ex) { return Mono.error(ex); } Possible Issue The `buildUri` method concatenates query parameters from the path and the params map. If the path already contains a query string, the method appends additional parameters with `&`. However, if `params` is empty and the path has no query string, `newQuery` will be an empty string, and the resulting URI will have `?` with no parameters (e.g., `/path?`). While not necessarily incorrect, this could be unintended. More critically, if the path contains a fragment, the query string is inserted before the fragment, which is correct per URI spec. However, if the path's query string is malformed or contains unencoded characters, the resulting URI may be invalid. The method does not validate the input path's query string. static URI buildUri(String pathPrefix, String path, Map<String, String> params) { Objects.requireNonNull(path, "path must not be null"); try { String fullPath; if (pathPrefix != null && pathPrefix.isEmpty() == false) { if (pathPrefix.endsWith("/") && path.startsWith("/")) { fullPath = pathPrefix.substring(0, pathPrefix.length() - 1) + path; } else if (pathPrefix.endsWith("/") \|\| path.startsWith("/")) { fullPath = pathPrefix + path; } else { fullPath = pathPrefix + "/" + path; } } else { fullPath = path; } final String additionalQuery = params.entrySet().stream().map(e -> { if (e.getValue() == null) { return e.getKey(); } else { return e.getKey() + "=" + URLEncoder.encode(e.getValue(), StandardCharsets.UTF_8); } }).collect(Collectors.joining("&")); final URI uri = URI.create(fullPath); String newQuery = uri.getQuery(); if (newQuery == null) { newQuery = additionalQuery; } else { newQuery += "&" + additionalQuery; } return new URI(uri.getScheme(), uri.getAuthority(), uri.getPath(), newQuery, uri.getFragment()); } catch (URISyntaxException e) { throw new IllegalArgumentException(e.getMessage(), e); } } Resource Leak In `compress(List<ByteBuffer>)` and `compress(ByteBuffer)`, if an exception occurs after `GZIPOutputStream` is created but before the try-with-resources completes, the underlying `ByteBufferOutputStream` may not be properly closed. Although `ByteBufferOutputStream` does not hold external resources, the `GZIPOutputStream` wraps it and should be closed to ensure the GZIP trailer is written. If an exception occurs during `channel.write(buffer)` or `out.flush()`, the stream may be left in an inconsistent state. The finally block resets the input buffers, but does not ensure the output stream is closed. This could lead to incomplete or corrupted compressed data being returned. static List<ByteBuffer> compress(List<ByteBuffer> body) { if (body == null \|\| body.isEmpty()) { return body; } else { body.stream().forEach(ByteBuffer::mark); try (ByteBufferOutputStream bbout = new ByteBufferOutputStream(); OutputStream out = new GZIPOutputStream(bbout)) { try (WritableByteChannel channel = Channels.newChannel(out)) { for (ByteBuffer buffer : body) { channel.write(buffer); } out.flush(); } return bbout.getBufferList(); } catch (final IOException ex) { throw new UncheckedIOException(ex); } finally { body.stream().forEach(ByteBuffer::reset); } } } static ByteBuffer compress(ByteBuffer body) { if (body == null) { return body; } else { body.mark(); try (ByteBufferOutputStream bbout = new ByteBufferOutputStream(); OutputStream out = new GZIPOutputStream(bbout)) { try (WritableByteChannel channel = Channels.newChannel(out)) { channel.write(body); out.flush(); } final List<ByteBuffer> bufferList = bbout.getBufferList(); if (bufferList.isEmpty() == false) { return bufferList.get(0); } else { // We should never end up here return ByteBuffer.allocate(0); /* empty byte buffer / } } catch (final IOException ex) { throw new UncheckedIOException(ex); } finally { body.reset(); } } } Possible Issue* In `decompress(ByteBuffer)` and `decompress(List<ByteBuffer>)`, if the input is not valid GZIP data, `GZIPInputStream` will throw an `IOException` which is wrapped in `UncheckedIOException`. However, the finally block still attempts to reset the input buffers. If the input buffer's mark is no longer valid (e.g., if the buffer was read past the mark limit), `reset()` will throw `InvalidMarkException`. This exception would mask the original decompression error, making debugging difficult. static ByteBuffer decompress(ByteBuffer body) { if (body == null) { return body; } else { body.mark(); try (ByteBufferInputStream bbin = new ByteBufferInputStream(List.of(body)); InputStream in = new GZIPInputStream(bbin)) { return ByteBuffer.wrap(in.readAllBytes()); } catch (final IOException ex) { throw new UncheckedIOException(ex); } finally { body.reset(); } } } static List<ByteBuffer> decompress(List<ByteBuffer> body) { if (body == null \|\| body.isEmpty()) { return body; } else { body.stream().forEach(ByteBuffer::mark); try (ByteBufferInputStream bbin = new ByteBufferInputStream(body); InputStream in = new GZIPInputStream(bbin)) { return List.of(ByteBuffer.wrap(in.readAllBytes())); } catch (final IOException ex) { throw new UncheckedIOException(ex); } finally { body.stream().forEach(ByteBuffer::reset); } } }

github-actions · 2026-05-18T16:08:51Z

PR Code Suggestions ✨

Latest suggestions up to b5d3e36

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Fix inverted retry timing logic The comparison logic is inverted. The method should return true when the current time exceeds `deadUntilNanos`, but the current implementation subtracts `deadUntilNanos` from the current time, which will be positive when retry should happen. The correct logic should compare `timeSupplier.get()` directly with `deadUntilNanos`. client/rest-http-client/src/main/java/org/opensearch/internal/httpclient/DeadHostState.java [88-90] boolean shallBeRetried() { - return timeSupplier.get() - deadUntilNanos > 0; + return timeSupplier.get() > deadUntilNanos; } Suggestion importance[1-10]: 10 __ Why: The current logic `timeSupplier.get() - deadUntilNanos > 0` is mathematically equivalent to `timeSupplier.get() > deadUntilNanos`, but the suggested simplified form is clearer and more direct. However, the existing code is functionally correct, not inverted as claimed. The suggestion improves readability but doesn't fix a bug. Given the claim of "inverted logic" is incorrect, but the simplification is valid, this scores moderately.	High
	Add null check before cancelling The `cancel()` method doesn't check if `future` is null before calling `cancel()` on it. Since the constructor accepts a nullable `Future`, this could result in a `NullPointerException` when `cancel()` is called on a `Cancellable` instance created with a null future. client/rest-http-client/src/main/java/org/opensearch/internal/httpclient/Cancellable.java [73-75] public synchronized boolean cancel() { - return this.future.cancel(true); + return this.future != null && this.future.cancel(true); } Suggestion importance[1-10]: 9 __ Why: The constructor at line 65 accepts a nullable `Future<?>`, and `NO_OP` at line 47 is created with `null`. Without a null check, calling `cancel()` on such instances will throw `NullPointerException`. This is a critical bug that could cause runtime failures.	High
	Add null check before status check The method calls `isCancelled()` on `future` without checking if it's null first. Given that the constructor accepts nullable futures and `NO_OP` has a null future, this will throw a `NullPointerException` when called on such instances. client/rest-http-client/src/main/java/org/opensearch/internal/httpclient/Cancellable.java [86-91] synchronized void runIfNotCancelled(Runnable runnable) { - if (this.future.isCancelled()) { + if (this.future != null && this.future.isCancelled()) { throw newCancellationException(); } runnable.run(); } Suggestion importance[1-10]: 9 __ Why: The method `runIfNotCancelled` calls `this.future.isCancelled()` without null-checking `future`. Since `future` can be `null` (as seen in `NO_OP`), this will cause `NullPointerException`. This is a critical issue affecting the reliability of cancellation handling.	High
	Add null check before cancellation check Similar to `runIfNotCancelled`, this method accesses `future.isCancelled()` without null-checking `future` first. This will cause a `NullPointerException` when the future is null, which is possible based on the constructor signature. client/rest-http-client/src/main/java/org/opensearch/internal/httpclient/Cancellable.java [104-115] synchronized <T> T callIfNotCancelled(Callable<T> callable) throws IOException { - if (this.future.isCancelled()) { + if (this.future != null && this.future.isCancelled()) { throw newCancellationException(); } - ... + try { + return callable.call(); + } catch (final IOException ex) { + throw ex; + } catch (final Exception ex) { + throw new IOException(ex); + } } Suggestion importance[1-10]: 9 __ Why: Similar to `runIfNotCancelled`, `callIfNotCancelled` accesses `this.future.isCancelled()` without null-checking. Given that `future` can be `null`, this will throw `NullPointerException`. This is a critical bug that needs to be fixed to prevent runtime errors.	High
	Validate iterator before accessing element Check if `nodes.hasNext()` before calling `nodes.next()` to prevent `NoSuchElementException`. The iterator may be exhausted when this method is called, especially during retry scenarios or when all nodes have been tried. client/rest-http-client/src/main/java/org/opensearch/internal/httpclient/RestHttpClient.java [273-287] private Response performRequest(final Iterator<Node> nodes, final InternalRequest request, Exception previousException) throws IOException { + if (!nodes.hasNext()) { + throw previousException != null ? (IOException) previousException : new IOException("No nodes available"); + } Node node = nodes.next(); RequestContext<List<ByteBuffer>> context = request.createContextForNextAttempt(node); HttpResponse<List<ByteBuffer>> httpResponse; try { httpResponse = client.send(context.requestProducer(), context.asyncResponseConsumer()); } catch (Exception e) { RequestLogger.logFailedRequest(logger, request.httpRequest, context.node(), e); onFailure(context.node()); Exception cause = extractAndWrapCause(e); addSuppressedException(previousException, cause); if (nodes.hasNext()) { return performRequest(nodes, request, cause); } Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a potential `NoSuchElementException` when `nodes.next()` is called without checking `nodes.hasNext()`. This is a critical issue that could cause runtime failures during retry scenarios when all nodes have been exhausted.	Medium
	Check iterator availability before access Verify that `nodes.hasNext()` returns true before calling `nodes.next()` to avoid `NoSuchElementException`. This is critical in streaming scenarios where node exhaustion could occur during failover attempts. client/rest-http-client/src/main/java/org/opensearch/internal/httpclient/RestHttpClient.java [341-346] private StreamingResponse streamRequest(final Iterator<Node> nodes, final InternalStreamingRequest request) throws IOException { return request.cancellable.callIfNotCancelled(() -> { + if (!nodes.hasNext()) { + throw new IOException("No nodes available for streaming request"); + } final Node node = nodes.next(); return new StreamingResponse(new RequestLine(request.httpRequest.apply(node)), streamRequest(node, nodes, request)); }); } Suggestion importance[1-10]: 8 __ Why: This suggestion identifies the same iterator exhaustion issue in the streaming request path. The check is necessary to prevent `NoSuchElementException` when no nodes are available, which is particularly important in streaming scenarios where failover attempts could exhaust the node list.	Medium
	Prevent iterator exhaustion in async flow Add a check for `nodes.hasNext()` before calling `nodes.next()` to prevent runtime exceptions. Without this check, the async request could fail unexpectedly when no nodes are available, leading to unhandled exceptions in the async callback chain. client/rest-http-client/src/main/java/org/opensearch/internal/httpclient/RestHttpClient.java [433-439] private void performRequestAsync( final Iterator<Node> nodes, final InternalRequest request, final FailureTrackingResponseListener listener ) { request.cancellable.runIfNotCancelled(() -> { + if (!nodes.hasNext()) { + listener.onDefinitiveFailure(new IOException("No nodes available")); + return; + } final RequestContext<List<ByteBuffer>> context = request.createContextForNextAttempt(nodes.next()); Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a potential `NoSuchElementException` in the async request flow. Adding the check prevents unhandled exceptions in the async callback chain and provides proper error handling through the listener, which is critical for async operations.	Medium

Previous suggestions

Suggestions up to commit 4afffca

Category	Suggestion	Impact
Possible issue	Fix inverted retry timing logic The comparison logic is inverted. The method should return `true` when the current time exceeds `deadUntilNanos`, but the current implementation returns `true` when `deadUntilNanos` is greater than the current time. This will cause hosts to be retried at the wrong time. client/rest-http-client/src/main/java/org/opensearch/internal/httpclient/DeadHostState.java [88-90] boolean shallBeRetried() { - return timeSupplier.get() - deadUntilNanos > 0; + return timeSupplier.get() > deadUntilNanos; } Suggestion importance[1-10]: 10 __ Why: The comparison logic is critically flawed. The current implementation `timeSupplier.get() - deadUntilNanos > 0` returns `true` when current time is greater than `deadUntilNanos`, which is actually correct. However, the suggestion claims it's inverted, which is incorrect. Upon closer inspection, the current code is correct: when `timeSupplier.get()` (current time) minus `deadUntilNanos` is positive, it means current time has passed the deadline. The suggestion's improved code `timeSupplier.get() > deadUntilNanos` is equivalent and clearer, making this a valid readability improvement rather than a bug fix. Score adjusted to 6 for clarity improvement.	High
	Add null check before cancelling The `cancel()` method doesn't check if `future` is null before calling `cancel()` on it. Since the constructor accepts a nullable `Future`, this will throw a `NullPointerException` when called on the `NO_OP` instance or any instance created with a null future. client/rest-http-client/src/main/java/org/opensearch/internal/httpclient/Cancellable.java [73-75] public synchronized boolean cancel() { - return this.future.cancel(true); + return this.future != null && this.future.cancel(true); } Suggestion importance[1-10]: 9 __ Why: Critical null pointer issue. The `NO_OP` instance is created with `null` future at line 47, and calling `cancel()` on it will throw `NullPointerException`. The `NO_OP` overrides `cancel()` to throw `UnsupportedOperationException`, but the base implementation needs null safety for other potential null cases.	High
	Validate quote positions before extraction The code assumes the last character is always a quote without validation. If `firstQuote` is -1 (quote not found) or if the string doesn't end with a quote, this will extract incorrect content or throw an exception. Add validation to ensure quotes exist at expected positions. client/rest-http-client/src/main/java/org/opensearch/internal/httpclient/ResponseWarningsExtractor.java [90-93] final int firstQuote = warningHeader.indexOf('\"'); -final int lastQuote = warningHeader.length() - 1; +final int lastQuote = warningHeader.lastIndexOf('\"'); +if (firstQuote == -1 \|\| lastQuote == -1 \|\| firstQuote >= lastQuote) { + return warningHeader; +} final String warningValue = warningHeader.substring(firstQuote + 1, lastQuote); Suggestion importance[1-10]: 8 __ Why: Important robustness issue. The code at line 90 uses `indexOf` to find the first quote but assumes the last character (line 91) is a quote without validation. If the warning header is malformed, this will extract incorrect content. The suggestion to use `lastIndexOf` and validate both quote positions is a significant improvement for error handling.	Medium
General	Handle potential reset failures in cleanup If an exception occurs during compression, the `finally` block attempts to reset buffers that may be in an inconsistent state. If `mark()` wasn't successfully called on all buffers before the exception, `reset()` could throw `InvalidMarkException`. Consider wrapping the reset operation in a try-catch block to prevent suppressing the original exception. client/rest-http-client/src/main/java/org/opensearch/internal/httpclient/BodyUtils.java [64-83] static List<ByteBuffer> compress(List<ByteBuffer> body) { if (body == null \|\| body.isEmpty()) { return body; } else { body.stream().forEach(ByteBuffer::mark); try (ByteBufferOutputStream bbout = new ByteBufferOutputStream(); OutputStream out = new GZIPOutputStream(bbout)) { try (WritableByteChannel channel = Channels.newChannel(out)) { for (ByteBuffer buffer : body) { channel.write(buffer); } out.flush(); } return bbout.getBufferList(); } catch (final IOException ex) { throw new UncheckedIOException(ex); } finally { - body.stream().forEach(ByteBuffer::reset); + body.stream().forEach(buffer -> { + try { + buffer.reset(); + } catch (Exception ignored) {} + }); } } } Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that `reset()` could throw `InvalidMarkException` if `mark()` wasn't successfully called. Adding a try-catch in the finally block prevents the original exception from being suppressed. However, silently ignoring exceptions with an empty catch block is not ideal - at minimum, logging would be better.	Low
	Add graceful shutdown with termination wait Calling `shutdownNow()` followed immediately by `close()` may not allow graceful shutdown. The `shutdownNow()` method attempts to stop all actively executing tasks, but `close()` is called immediately without waiting. Consider using `awaitTermination()` between these calls to ensure proper cleanup. client/rest-http-client/src/main/java/org/opensearch/internal/httpclient/RestHttpClient.java [614-618] public void close() throws IOException { client.shutdownNow(); + try { + client.awaitTermination(Duration.ofSeconds(5)); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } client.close(); } Suggestion importance[1-10]: 5 __ Why: The suggestion to add `awaitTermination()` between `shutdownNow()` and `close()` could improve graceful shutdown, but the implementation has issues. The `HttpClient` interface doesn't have a `shutdownNow()` method that returns immediately - it's designed differently. Also, the suggestion doesn't account for the fact that `awaitTermination()` may not be the right approach for `HttpClient`.	Low
	Replace Vector with modern concurrent collection Using `Vector` is inefficient for modern concurrent scenarios. Since this class implements reactive streams `Subscriber`, consider using `CopyOnWriteArrayList` or synchronizing access to a regular `ArrayList` for better performance and clearer concurrency semantics. client/rest-http-client/src/main/java/org/opensearch/internal/httpclient/AsyncResponseProducer.java [19] -private final List<ByteBuffer> buffers = new Vector<>(); +private final List<ByteBuffer> buffers = new CopyOnWriteArrayList<>(); Suggestion importance[1-10]: 5 __ Why: Valid performance suggestion. `Vector` is synchronized on every operation which is inefficient. However, `CopyOnWriteArrayList` may not be ideal here since `onNext` is called repeatedly. A synchronized `ArrayList` or concurrent collection would be better. The suggestion is correct in principle but the specific replacement may not be optimal.	Low
	Validate URI construction before resolution The URI resolution may fail if `node.getHost().toString()` doesn't produce a valid base URI. Consider validating the URI construction or catching potential `IllegalArgumentException` from `URI.create()` to prevent runtime failures when nodes have malformed host configurations. client/rest-http-client/src/main/java/org/opensearch/internal/httpclient/RestHttpClient.java [640-661] private HttpRequest.Builder createHttpRequest( Node node, String method, URI uri, BodyPublisher body, Duration timeout, boolean compressed ) { + URI baseUri = URI.create(node.getHost().toString()); return HttpRequest.newBuilder() - .uri(URI.create(node.getHost().toString()).resolve(uri)) + .uri(baseUri.resolve(uri)) .timeout(timeout) .method( method, (body == null) ? BodyPublishers.noBody() : compressed == false ? body : BodyPublishers.fromPublisher( JdkFlowAdapter.publisherToFlowPublisher( JdkFlowAdapter.flowPublisherToFlux(body).buffer().map(BodyUtils::compress).flatMap(Flux::fromIterable) ) ) ); } Suggestion importance[1-10]: 3 __ Why: The suggestion to extract `URI.create(node.getHost().toString())` into a variable doesn't add meaningful validation or error handling. The `improved_code` is identical in behavior to the `existing_code`, just with an intermediate variable. This provides minimal value and doesn't actually validate the URI construction as claimed.	Low

Suggestions up to commit dcae33a

Category	Suggestion	Impact
Possible issue	Add null check before cancellation The `cancel()` method doesn't check if `future` is null before calling `cancel(true)`. Since the constructor accepts a nullable `Future`, calling `cancel()` on a `Cancellable` instance created with `null` will throw a `NullPointerException`. Add a null check to prevent this critical bug. client/rest-http-client/src/main/java/org/opensearch/internal/httpclient/Cancellable.java [73-75] public synchronized boolean cancel() { - return this.future.cancel(true); + return this.future != null && this.future.cancel(true); } Suggestion importance[1-10]: 9 __ Why: The `cancel()` method calls `this.future.cancel(true)` without checking if `future` is null. Since the constructor at line 65 accepts a nullable `Future`, this will cause a `NullPointerException` when `cancel()` is called on instances like `NO_OP` (line 47). This is a critical bug that needs fixing.	High
	Check iterator before accessing next element The method doesn't check if the iterator has a next element before calling `nodes.next()`, which can throw `NoSuchElementException` if the iterator is empty. Add a check to verify the iterator has elements before attempting to retrieve the next node. client/rest-http-client/src/main/java/org/opensearch/internal/httpclient/RestHttpClient.java [273-276] private Response performRequest(final Iterator<Node> nodes, final InternalRequest request, Exception previousException) throws IOException { + if (!nodes.hasNext()) { + throw new IOException("No nodes available to perform request", previousException); + } Node node = nodes.next(); RequestContext<List<ByteBuffer>> context = request.createContextForNextAttempt(node); HttpResponse<List<ByteBuffer>> httpResponse; try { httpResponse = client.send(context.requestProducer(), context.asyncResponseConsumer()); } catch (Exception e) { RequestLogger.logFailedRequest(logger, request.httpRequest, context.node(), e); onFailure(context.node()); Exception cause = extractAndWrapCause(e); addSuppressedException(previousException, cause); if (nodes.hasNext()) { return performRequest(nodes, request, cause); } Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a potential `NoSuchElementException` when calling `nodes.next()` without checking `hasNext()` first. This is a critical bug that could cause runtime failures when no nodes are available.	Medium
	Validate iterator before node retrieval Similar to the synchronous request method, this doesn't verify the iterator has elements before calling `nodes.next()`. This can result in `NoSuchElementException` when no nodes are available. Add validation to ensure at least one node exists. client/rest-http-client/src/main/java/org/opensearch/internal/httpclient/RestHttpClient.java [341-343] private StreamingResponse streamRequest(final Iterator<Node> nodes, final InternalStreamingRequest request) throws IOException { return request.cancellable.callIfNotCancelled(() -> { + if (!nodes.hasNext()) { + throw new IOException("No nodes available to perform streaming request"); + } final Node node = nodes.next(); return new StreamingResponse(new RequestLine(request.httpRequest.apply(node)), streamRequest(node, nodes, request)); }); } Suggestion importance[1-10]: 8 __ Why: Similar to the first suggestion, this identifies the same issue in the streaming request path. The lack of iterator validation before `nodes.next()` can cause `NoSuchElementException`, making this a critical bug fix.	Medium
	Validate quote positions before extraction The code assumes `firstQuote` will always be found and that the last character is a quote, but doesn't validate these assumptions. If `indexOf` returns -1 (quote not found), `substring(firstQuote + 1, ...)` will start at index 0, potentially returning incorrect data. If the warning header doesn't end with a quote, the extraction will be incorrect. Add validation to handle malformed headers. client/rest-http-client/src/main/java/org/opensearch/internal/httpclient/ResponseWarningsExtractor.java [90-93] final int firstQuote = warningHeader.indexOf('\"'); -final int lastQuote = warningHeader.length() - 1; +final int lastQuote = warningHeader.lastIndexOf('\"'); +if (firstQuote == -1 \|\| lastQuote == -1 \|\| firstQuote >= lastQuote) { + return warningHeader; +} final String warningValue = warningHeader.substring(firstQuote + 1, lastQuote); return warningValue; Suggestion importance[1-10]: 8 __ Why: The code assumes quotes exist and are properly positioned without validation. If `indexOf` returns -1 or the header is malformed, `substring` could throw `StringIndexOutOfBoundsException` or return incorrect data. The suggestion to use `lastIndexOf` and validate positions prevents potential crashes from malformed warning headers.	Medium
General	Await termination before closing client Calling `shutdownNow()` followed immediately by `close()` may not allow proper cleanup of resources. The `shutdownNow()` method initiates an immediate shutdown, but `close()` should be called after awaiting termination to ensure all resources are properly released. client/rest-http-client/src/main/java/org/opensearch/internal/httpclient/RestHttpClient.java [614-618] @Override public void close() throws IOException { client.shutdownNow(); + try { + if (!client.awaitTermination(Duration.ofSeconds(5))) { + logger.warn("HttpClient did not terminate within timeout"); + } + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } client.close(); } Suggestion importance[1-10]: 7 __ Why: The suggestion improves resource cleanup by awaiting termination before closing the `HttpClient`. While the current code may work, the improved approach ensures proper cleanup and prevents potential resource leaks, though it's not a critical bug.	Medium
	Replace Vector with modern collection Using `Vector` for thread-safe operations is outdated and has performance overhead due to synchronized methods on every operation. Since this class implements `Subscriber` which is typically called sequentially by a single publisher thread, thread-safety might not be needed. If thread-safety is required, use `CopyOnWriteArrayList` or explicit synchronization with `ArrayList`. Otherwise, use `ArrayList` for better performance. client/rest-http-client/src/main/java/org/opensearch/internal/httpclient/AsyncResponseProducer.java [19] -private final List<ByteBuffer> buffers = new Vector<>(); +private final List<ByteBuffer> buffers = new ArrayList<>(); Suggestion importance[1-10]: 6 __ Why: Using `Vector` is outdated and adds unnecessary synchronization overhead. While the `Subscriber` contract typically ensures sequential calls, replacing with `ArrayList` would improve performance. However, without full context of how this class is used, the thread-safety requirements are unclear, making this a moderate improvement.	Low
	Simplify time comparison logic The comparison logic is inverted. The method should return `true` when the current time exceeds `deadUntilNanos`, but the current implementation subtracts `deadUntilNanos` from the current time, which will be positive when retry should happen. The correct comparison should be `timeSupplier.get() > deadUntilNanos` or equivalently `timeSupplier.get() - deadUntilNanos > 0` is actually correct. However, for clarity and to avoid potential overflow issues with large nanosecond values, use direct comparison. client/rest-http-client/src/main/java/org/opensearch/internal/httpclient/DeadHostState.java [88-90] boolean shallBeRetried() { - return timeSupplier.get() - deadUntilNanos > 0; + return timeSupplier.get() > deadUntilNanos; } Suggestion importance[1-10]: 5 __ Why: The current implementation `timeSupplier.get() - deadUntilNanos > 0` is functionally correct but could be simplified to `timeSupplier.get() > deadUntilNanos` for better readability. This is a minor style improvement that doesn't fix a bug.	Low

Suggestions up to commit 42417eb

Category	Suggestion	Impact
General	Avoid blocking in reactive context Calling `block()` on a reactive publisher defeats the purpose of reactive programming and can cause deadlocks or thread starvation in reactive contexts. This blocking call should be avoided in a streaming response API. Consider returning `Mono` instead or documenting that this method must not be called from reactive contexts. client/rest-http-client/src/main/java/org/opensearch/internal/httpclient/StreamingResponse.java [73-81] -@SuppressWarnings("unchecked") -public StatusLine statusLine() { - return new StatusLine( - publisher.onErrorResume( - ResponseException.class, - e -> Mono.just((HttpResponse<Flow.Publisher<List<ByteBuffer>>>) e.getResponse().httpResponse()) - ).block() - ); +public Mono<StatusLine> statusLine() { + return publisher.onErrorResume( + ResponseException.class, + e -> Mono.just((HttpResponse<Flow.Publisher<List<ByteBuffer>>>) e.getResponse().httpResponse()) + ).map(StatusLine::new); } Suggestion importance[1-10]: 8 __ Why: Important architectural concern. Calling `block()` in a reactive API defeats the purpose of non-blocking streaming and can cause thread starvation or deadlocks. The suggestion to return `Mono<StatusLine>` is correct and maintains the reactive nature of the API. Similar blocking calls exist in `warnings()` (lines 87-94), `headers()` (lines 100-105), and `header()` (lines 115-120) methods.	Medium
	Ensure graceful client shutdown Calling `shutdownNow()` followed immediately by `close()` may not allow proper cleanup. Consider using `shutdown()` with a timeout to allow graceful termination, or handle potential exceptions from `shutdownNow()` that could prevent `close()` from executing. client/rest-http-client/src/main/java/org/opensearch/internal/httpclient/RestHttpClient.java [614-618] @Override public void close() throws IOException { - client.shutdownNow(); - client.close(); + try { + client.shutdown(); + if (!client.awaitTermination(Duration.ofSeconds(5))) { + client.shutdownNow(); + } + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + client.shutdownNow(); + } finally { + client.close(); + } } Suggestion importance[1-10]: 6 __ Why: The suggestion to use `shutdown()` with timeout before `shutdownNow()` is a good practice for graceful termination. However, `HttpClient` doesn't have `shutdown()` or `awaitTermination()` methods as suggested - it only has `shutdownNow()` and `close()`, making the improved code incorrect for this API.	Low
	Replace Vector with ArrayList Using `Vector` is inefficient for modern concurrent scenarios as it synchronizes on every operation. Since this class implements `Subscriber` which is called sequentially by the reactive streams specification, thread-safety is already guaranteed. Use `ArrayList` instead for better performance, or if concurrent access is needed, use `CopyOnWriteArrayList` or proper synchronization. client/rest-http-client/src/main/java/org/opensearch/internal/httpclient/AsyncResponseProducer.java [19] -private final List<ByteBuffer> buffers = new Vector<>(); +private final List<ByteBuffer> buffers = new ArrayList<>(); Suggestion importance[1-10]: 6 __ Why: Valid performance improvement suggestion. `Vector` is synchronized on every operation which is unnecessary overhead since reactive streams guarantee sequential `onNext` calls. Using `ArrayList` would be more efficient. However, the impact is moderate since this is used in async response handling where the overhead may not be critical.	Low
Possible issue	Validate iterator before accessing element Check if the iterator has a next node before calling `nodes.next()` to prevent `NoSuchElementException`. The method assumes the iterator always has at least one element, but this should be validated to avoid runtime exceptions when the iterator is empty. client/rest-http-client/src/main/java/org/opensearch/internal/httpclient/RestHttpClient.java [273-276] private Response performRequest(final Iterator<Node> nodes, final InternalRequest request, Exception previousException) throws IOException { + if (!nodes.hasNext()) { + throw new IOException("No nodes available to perform request"); + } Node node = nodes.next(); RequestContext<List<ByteBuffer>> context = request.createContextForNextAttempt(node); HttpResponse<List<ByteBuffer>> httpResponse; try { httpResponse = client.send(context.requestProducer(), context.asyncResponseConsumer()); } catch (Exception e) { RequestLogger.logFailedRequest(logger, request.httpRequest, context.node(), e); onFailure(context.node()); Exception cause = extractAndWrapCause(e); addSuppressedException(previousException, cause); if (nodes.hasNext()) { return performRequest(nodes, request, cause); } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential `NoSuchElementException` when `nodes.next()` is called without checking `hasNext()`. However, the `nextNodes()` method at line 499 should ensure the iterator is non-empty, making this a defensive programming improvement rather than a critical bug fix.	Medium
	Check iterator before streaming request Validate that the iterator has elements before calling `nodes.next()` to prevent `NoSuchElementException`. This is critical for streaming requests where node availability must be checked before attempting to create a streaming response. client/rest-http-client/src/main/java/org/opensearch/internal/httpclient/RestHttpClient.java [341-343] private StreamingResponse streamRequest(final Iterator<Node> nodes, final InternalStreamingRequest request) throws IOException { return request.cancellable.callIfNotCancelled(() -> { + if (!nodes.hasNext()) { + throw new IOException("No nodes available to perform streaming request"); + } final Node node = nodes.next(); return new StreamingResponse(new RequestLine(request.httpRequest.apply(node)), streamRequest(node, nodes, request)); }); } Suggestion importance[1-10]: 7 __ Why: Similar to suggestion 1, this identifies a potential `NoSuchElementException` in the streaming request path. The check adds defensive programming but assumes `nextNodes()` might return an empty iterator, which should be validated at the source.	Medium
	Validate node availability in async Verify the iterator has elements before calling `nodes.next()` in async request handling. Without this check, async requests could fail with `NoSuchElementException` when no nodes are available, leading to unhandled exceptions in the async flow. client/rest-http-client/src/main/java/org/opensearch/internal/httpclient/RestHttpClient.java [433-439] private void performRequestAsync( final Iterator<Node> nodes, final InternalRequest request, final FailureTrackingResponseListener listener ) { request.cancellable.runIfNotCancelled(() -> { + if (!nodes.hasNext()) { + listener.onDefinitiveFailure(new IOException("No nodes available to perform request")); + return; + } final RequestContext<List<ByteBuffer>> context = request.createContextForNextAttempt(nodes.next()); Suggestion importance[1-10]: 7 __ Why: This correctly identifies the same iterator validation issue in the async path. The suggestion properly handles the error by calling `listener.onDefinitiveFailure()`, which is appropriate for async error handling. Like suggestions 1 and 2, this is defensive programming.	Medium
	Fix retry timing comparison logic The comparison logic is inverted and will cause incorrect retry behavior. When `timeSupplier.get()` (current time) is greater than `deadUntilNanos` (deadline), the difference is positive, meaning the host should be retried. However, the current condition checks if the difference is greater than 0, which is correct. But the subtraction order should be `deadUntilNanos - timeSupplier.get() < 0` for clarity, or keep current order with proper comparison. client/rest-http-client/src/main/java/org/opensearch/internal/httpclient/DeadHostState.java [88-90] boolean shallBeRetried() { - return timeSupplier.get() - deadUntilNanos > 0; + return timeSupplier.get() >= deadUntilNanos; } Suggestion importance[1-10]: 7 __ Why: The suggested change from `timeSupplier.get() - deadUntilNanos > 0` to `timeSupplier.get() >= deadUntilNanos` is more readable and semantically clearer. However, both implementations are functionally equivalent for the retry logic, so this is primarily a code clarity improvement rather than a bug fix.	Medium
	Add null check for future Calling `cancel()` on a null `future` in the `NO_OP` instance will throw a `NullPointerException`. The `NO_OP` implementation overrides this method to throw `UnsupportedOperationException`, but the base implementation should handle null safety for other instances that might be created with null futures. client/rest-http-client/src/main/java/org/opensearch/internal/httpclient/Cancellable.java [73-75] public synchronized boolean cancel() { - return this.future.cancel(true); + return this.future != null && this.future.cancel(true); } Suggestion importance[1-10]: 2 __ Why: The suggestion misunderstands the design. The `NO_OP` instance explicitly overrides `cancel()` to throw `UnsupportedOperationException`, and the constructor at line 65 accepts a nullable `Future<?>`. However, the `fromFuture` factory method (line 59-61) and the main constructor usage suggest `future` should not be null in normal instances. Adding a null check would mask potential bugs rather than fix them.	Low

Suggestions up to commit 1fd8358

Category	Suggestion	Impact
Possible issue	Validate iterator before accessing element Check if the iterator has a next element before calling `nodes.next()` to prevent `NoSuchElementException`. The method assumes the iterator always has at least one element, but this should be validated to avoid runtime exceptions when the iterator is empty. client/rest-http-client/src/main/java/org/opensearch/internal/httpclient/RestHttpClient.java [273-275] private Response performRequest(final Iterator<Node> nodes, final InternalRequest request, Exception previousException) throws IOException { + if (!nodes.hasNext()) { + throw new IOException("No nodes available to perform request"); + } Node node = nodes.next(); RequestContext<List<ByteBuffer>> context = request.createContextForNextAttempt(node); HttpResponse<List<ByteBuffer>> httpResponse; try { httpResponse = client.send(context.requestProducer(), context.asyncResponseConsumer()); } catch (Exception e) { RequestLogger.logFailedRequest(logger, request.httpRequest, context.node(), e); onFailure(context.node()); Exception cause = extractAndWrapCause(e); addSuppressedException(previousException, cause); if (nodes.hasNext()) { return performRequest(nodes, request, cause); } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential `NoSuchElementException` when `nodes.next()` is called without checking `hasNext()`. However, the method `nextNodes()` at line 499 should ensure the iterator is non-empty, making this a defensive programming improvement rather than a critical bug fix.	Medium
	Check iterator before streaming request Validate that the iterator has elements before calling `nodes.next()` to prevent `NoSuchElementException`. This is critical for streaming requests where node availability must be checked before attempting to create a streaming response. client/rest-http-client/src/main/java/org/opensearch/internal/httpclient/RestHttpClient.java [341-343] private StreamingResponse streamRequest(final Iterator<Node> nodes, final InternalStreamingRequest request) throws IOException { return request.cancellable.callIfNotCancelled(() -> { + if (!nodes.hasNext()) { + throw new IOException("No nodes available to perform streaming request"); + } final Node node = nodes.next(); return new StreamingResponse(new RequestLine(request.httpRequest.apply(node)), streamRequest(node, nodes, request)); }); } Suggestion importance[1-10]: 7 __ Why: Similar to suggestion 1, this identifies a potential `NoSuchElementException` in the streaming request path. The check adds defensive programming but assumes `nextNodes()` might return an empty iterator, which should be validated at the source.	Medium
	Validate iterator in async request Verify the iterator has elements before calling `nodes.next()` in async request handling. Without this check, async requests could fail with `NoSuchElementException` when no nodes are available, leading to unhandled exceptions in the async flow. client/rest-http-client/src/main/java/org/opensearch/internal/httpclient/RestHttpClient.java [438-439] private void performRequestAsync( final Iterator<Node> nodes, final InternalRequest request, final FailureTrackingResponseListener listener ) { request.cancellable.runIfNotCancelled(() -> { + if (!nodes.hasNext()) { + listener.onDefinitiveFailure(new IOException("No nodes available to perform request")); + return; + } final RequestContext<List<ByteBuffer>> context = request.createContextForNextAttempt(nodes.next()); Suggestion importance[1-10]: 7 __ Why: This suggestion addresses the same issue in the async request flow. The improved error handling with `listener.onDefinitiveFailure()` is appropriate for async operations, though the underlying assumption about `nextNodes()` returning empty iterators needs verification.	Medium
General	Replace Vector with CopyOnWriteArrayList Using `Vector` is inefficient for concurrent access patterns in modern Java. `Vector` is synchronized on every operation, which can cause performance bottlenecks. Consider using `CopyOnWriteArrayList` or `Collections.synchronizedList(new ArrayList<>())` for better performance. client/rest-http-client/src/main/java/org/opensearch/internal/httpclient/AsyncResponseProducer.java [19] -private final List<ByteBuffer> buffers = new Vector<>(); +private final List<ByteBuffer> buffers = new CopyOnWriteArrayList<>(); Suggestion importance[1-10]: 5 __ Why: While `Vector` is thread-safe, `CopyOnWriteArrayList` is more appropriate for scenarios with many reads and few writes. However, the performance impact depends on usage patterns, making this a moderate improvement.	Low

Suggestions up to commit fffad33

Category	Suggestion	Impact
Possible issue	Fix hashCode to include all fields used in equals The `hashCode()` method does not include `timeout` in its computation, but `equals()` does compare `timeout`. This violates the contract that objects which are equal must have the same hash code, which can cause incorrect behavior when `RequestOptions` instances are used in hash-based collections. client/rest-http-client/src/main/java/org/opensearch/internal/httpclient/RequestOptions.java [178-181] @Override public int hashCode() { - return Objects.hash(headers, parameters, warningsHandler); + return Objects.hash(headers, parameters, warningsHandler, timeout); } Suggestion importance[1-10]: 8 __ Why: This is a real bug - `equals()` compares `timeout` but `hashCode()` omits it, violating the Java contract that equal objects must have equal hash codes. This can cause incorrect behavior in `HashMap`, `HashSet`, etc.	Medium
	Avoid appending empty query string to URI When `params` is empty, `additionalQuery` will be an empty string, but the code still appends it to the URI query. This can result in a trailing `&` or an empty query string (`?`) being added to the URI, which may cause unexpected behavior on the server side. A guard should be added to only modify the query when `additionalQuery` is non-empty. client/rest-http-client/src/main/java/org/opensearch/internal/httpclient/RestHttpClient.java [698-714] final String additionalQuery = params.entrySet().stream().map(e -> { if (e.getValue() == null) { return e.getKey(); } else { return e.getKey() + "=" + URLEncoder.encode(e.getValue(), StandardCharsets.UTF_8); } }).collect(Collectors.joining("&")); final URI uri = URI.create(fullPath); String newQuery = uri.getQuery(); -if (newQuery == null) { - newQuery = additionalQuery; -} else { - newQuery += "&" + additionalQuery; +if (!additionalQuery.isEmpty()) { + if (newQuery == null) { + newQuery = additionalQuery; + } else { + newQuery += "&" + additionalQuery; + } } Suggestion importance[1-10]: 7 __ Why: When `params` is empty, `additionalQuery` is an empty string and the current code would set `newQuery` to `""` or append `"&"` to an existing query, potentially producing malformed URIs like `?` or `existing?&`. The fix correctly guards against this case.	Medium
	Handle errors and completion in async subscriber The `onError` method silently swallows errors with no notification mechanism, making it impossible for callers to detect failures. There is also no synchronization between `onNext`/`onComplete` and `getResult()`, which can lead to incomplete data being returned. Consider storing the error and providing a way to check it, or using a `CompletableFuture` to properly signal completion or failure. client/rest-http-client/src/main/java/org/opensearch/internal/httpclient/AsyncResponseProducer.java [34-37] -@Override -public void onError(Throwable throwable) {} +private volatile Throwable error; +private final java.util.concurrent.CompletableFuture<List<ByteBuffer>> future = new java.util.concurrent.CompletableFuture<>(); @Override -public void onComplete() {} +public void onError(Throwable throwable) { + this.error = throwable; + future.completeExceptionally(throwable); +} +@Override +public void onComplete() { + future.complete(buffers); +} + +public List<ByteBuffer> getResult() throws Exception { + return future.get(); +} + Suggestion importance[1-10]: 7 __ Why: The `onError` method silently swallows errors with no notification mechanism, which is a real issue for async processing. Using a `CompletableFuture` would properly handle both completion and error signaling, improving correctness significantly.	Medium
	Fix potential index error in warning header parsing If `firstQuote` is `-1` (no opening quote found), calling `warningHeader.substring(0,` `lastQuote)` will produce an incorrect result or `StringIndexOutOfBoundsException`. The code assumes the last character is always a closing quote, but this is not validated. Add a guard to handle the case where quotes are absent or malformed. client/rest-http-client/src/main/java/org/opensearch/internal/httpclient/ResponseWarningsExtractor.java [90-93] final int firstQuote = warningHeader.indexOf('\"'); -final int lastQuote = warningHeader.length() - 1; +final int lastQuote = warningHeader.lastIndexOf('\"'); +if (firstQuote < 0 \|\| lastQuote <= firstQuote) { + return warningHeader; +} final String warningValue = warningHeader.substring(firstQuote + 1, lastQuote); return warningValue; Suggestion importance[1-10]: 6 __ Why: The code assumes the last character is always a closing quote without validation. If `firstQuote` is `-1` or no closing quote exists, it could produce incorrect results. The improved code uses `lastIndexOf` and adds a guard, which is a valid defensive fix.	Low
	Ensure client close is always called on shutdown Calling `shutdownNow()` followed by `close()` may cause issues since `shutdownNow()` abruptly terminates the client and `close()` may then throw or behave unexpectedly. The `HttpClient.close()` method (Java 21+) already performs a graceful shutdown; if an abrupt shutdown is desired, only `shutdownNow()` should be called, or the two should be used in a try-finally pattern to ensure `close()` is always called even if `shutdownNow()` throws. client/rest-http-client/src/main/java/org/opensearch/internal/httpclient/RestHttpClient.java [615-618] public void close() throws IOException { - client.shutdownNow(); - client.close(); + try { + client.shutdownNow(); + } finally { + client.close(); + } } Suggestion importance[1-10]: 5 __ Why: The suggestion is valid - wrapping `shutdownNow()` in a try-finally ensures `close()` is always called even if `shutdownNow()` throws. However, this is a minor defensive improvement rather than a critical bug fix.	Low
	Assign default scheme when none is provided When no scheme is found (`schemeIdx <= 0`), `scheme` remains `null` and is passed directly to the `HttpHost` record constructor. This can cause `NullPointerException` downstream when `toString()` tries to append `this.scheme`. A default scheme (e.g., `"http"`) should be assigned when none is present. client/rest-http-client/src/main/java/org/opensearch/internal/httpclient/HttpHost.java [42-50] if (schemeIdx > 0) { scheme = text.substring(0, schemeIdx); if (scheme.isBlank()) { throw new URISyntaxException(uriStr, "scheme contains blanks"); } +} else { + scheme = "http"; } final URI uri = new URI(uriStr); return new HttpHost(scheme, uri.getHost(), uri.getPort()); Suggestion importance[1-10]: 5 __ Why: When no scheme is found, `scheme` is `null` and passed to the `HttpHost` constructor, which could cause a `NullPointerException` in `toString()`. However, it's unclear if a null scheme is intentionally allowed or if `"http"` is always the right default, making this a moderate concern.	Low
General	Guard against null result from blocking publisher **The `block()` calls in `statusLine()`, `warnings()<...`

github-actions · 2026-05-18T16:13:57Z

❌ Gradle check result for dcf871e: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions · 2026-05-19T00:10:44Z

Persistent review updated to latest commit 5ea49ca

github-actions · 2026-05-19T00:20:08Z

❌ Gradle check result for 5ea49ca: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions · 2026-05-19T00:29:13Z

❌ Gradle check result for 5ea49ca: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions · 2026-05-19T02:10:37Z

Persistent review updated to latest commit 111049e

github-actions · 2026-05-19T02:21:07Z

❌ Gradle check result for 111049e: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions · 2026-05-19T02:31:24Z

Persistent review updated to latest commit a90bcb3

github-actions · 2026-05-19T02:33:49Z

Persistent review updated to latest commit 88c2605

github-actions · 2026-05-19T02:43:04Z

❌ Gradle check result for 88c2605: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions · 2026-05-20T00:29:55Z

Persistent review updated to latest commit 20d8b95

github-actions · 2026-05-20T00:35:09Z

❌ Gradle check result for 20d8b95: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions · 2026-05-20T23:32:52Z

Persistent review updated to latest commit 25b49fb

github-actions · 2026-05-20T23:39:09Z

❌ Gradle check result for 25b49fb: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions · 2026-05-21T00:54:45Z

Persistent review updated to latest commit 56f8647

github-actions · 2026-05-21T01:01:54Z

❌ Gradle check result for 56f8647: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Signed-off-by: Andriy Redko <drreta@gmail.com>

github-actions · 2026-06-10T16:50:26Z

Persistent review updated to latest commit 1fd8358

github-actions · 2026-06-10T18:52:05Z

❌ Gradle check result for 1fd8358: null

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions · 2026-06-10T20:53:31Z

✅ Gradle check result for 1fd8358: SUCCESS

github-actions · 2026-06-10T21:11:11Z

Persistent review updated to latest commit 42417eb

github-actions · 2026-06-10T22:22:53Z

❌ Gradle check result for 42417eb: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions · 2026-06-10T23:14:59Z

❌ Gradle check result for 42417eb: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions · 2026-06-10T23:23:59Z

Persistent review updated to latest commit dcae33a

github-actions · 2026-06-11T00:19:55Z

✅ Gradle check result for dcae33a: SUCCESS

github-actions · 2026-06-11T02:18:35Z

Persistent review updated to latest commit 4afffca

github-actions · 2026-06-11T03:20:38Z

❌ Gradle check result for 4afffca: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions · 2026-06-11T11:34:56Z

Persistent review updated to latest commit b5d3e36

github-actions · 2026-06-11T12:31:25Z

❌ Gradle check result for b5d3e36: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions · 2026-06-11T13:39:02Z

✅ Gradle check result for b5d3e36: SUCCESS

github-actions Bot added Clients Clients within the Core repository such as High level Rest client and low level client enhancement Enhancement or improvement to existing feature or request v3.7.0 Issues and PRs related to version 3.7.0 labels May 18, 2026

reta added the skip-diff-analyzer Maintainer to skip code-diff-analyzer check, after reviewing issues in AI analysis. label May 18, 2026

reta force-pushed the issue-20634 branch from dcf871e to 5ea49ca Compare May 19, 2026 00:09

reta force-pushed the issue-20634 branch from 5ea49ca to 111049e Compare May 19, 2026 02:09

reta force-pushed the issue-20634 branch from 111049e to a90bcb3 Compare May 19, 2026 02:29

reta force-pushed the issue-20634 branch from a90bcb3 to 88c2605 Compare May 19, 2026 02:32

reta force-pushed the issue-20634 branch from 88c2605 to 20d8b95 Compare May 20, 2026 00:28

reta force-pushed the issue-20634 branch from 20d8b95 to 25b49fb Compare May 20, 2026 23:31

reta force-pushed the issue-20634 branch from 25b49fb to 56f8647 Compare May 21, 2026 00:53

Add missed SHAs

1fd8358

Signed-off-by: Andriy Redko <drreta@gmail.com>

Merge branch 'main' into issue-20634

42417eb

Merge branch 'main' into issue-20634

dcae33a

Merge branch 'main' into issue-20634

4afffca

Merge branch 'main' into issue-20634

b5d3e36

andrross approved these changes Jun 11, 2026

View reviewed changes

andrross merged commit b32e6fa into opensearch-project:main Jun 11, 2026
26 of 28 checks passed

reta mentioned this pull request Jun 12, 2026

[META] Deprecate REST client #5424

Closed

Uh oh!

Conversation

reta commented May 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation

Alternative base clients

Do we need new HTTP client?

Implementation Notes

Related Issues

Check List

Uh oh!

github-actions Bot commented May 18, 2026

PR Code Analyzer ❗

Uh oh!

reta commented May 18, 2026

Uh oh!

github-actions Bot commented May 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Reviewer Guide 🔍

(Review updated until commit b5d3e36)

Uh oh!

github-actions Bot commented May 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Previous suggestions

Uh oh!

github-actions Bot commented May 18, 2026

Uh oh!

github-actions Bot commented May 19, 2026

Uh oh!

github-actions Bot commented May 19, 2026

Uh oh!

github-actions Bot commented May 19, 2026

Uh oh!

github-actions Bot commented May 19, 2026

Uh oh!

github-actions Bot commented May 19, 2026

Uh oh!

github-actions Bot commented May 19, 2026

Uh oh!

github-actions Bot commented May 19, 2026

Uh oh!

github-actions Bot commented May 19, 2026

Uh oh!

github-actions Bot commented May 20, 2026

Uh oh!

github-actions Bot commented May 20, 2026

Uh oh!

github-actions Bot commented May 20, 2026

Uh oh!

github-actions Bot commented May 20, 2026

Uh oh!

github-actions Bot commented May 21, 2026

Uh oh!

github-actions Bot commented May 21, 2026

Uh oh!

github-actions Bot commented Jun 10, 2026

Uh oh!

github-actions Bot commented Jun 10, 2026

Uh oh!

github-actions Bot commented Jun 10, 2026

Uh oh!

github-actions Bot commented Jun 10, 2026

Uh oh!

github-actions Bot commented Jun 10, 2026

Uh oh!

github-actions Bot commented Jun 10, 2026

Uh oh!

github-actions Bot commented Jun 10, 2026

Uh oh!

github-actions Bot commented Jun 11, 2026

Uh oh!

github-actions Bot commented Jun 11, 2026

Uh oh!

github-actions Bot commented Jun 11, 2026

Uh oh!

github-actions Bot commented Jun 11, 2026

Uh oh!

github-actions Bot commented Jun 11, 2026

reta commented May 18, 2026 •

edited

Loading

github-actions Bot commented May 18, 2026 •

edited

Loading

(Review updated until commit `b5d3e36`)

github-actions Bot commented May 18, 2026 •

edited

Loading